Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Refactor moves that call a random move (Metronome, Assist, Sleep Talk, Copycat, Mirror Move) #3380

Open
wants to merge 62 commits into
base: beta
Choose a base branch
from

Conversation

schmidtc1
Copy link
Contributor

@schmidtc1 schmidtc1 commented Aug 6, 2024

Rebranch of #3075

TODO

  • Prevent allies being targets of Metronome/Copycat/other random move targetting
    • Verify that self-status/Aromatic Mist still can target allies
  • Fix issue of two-turn moves with Metronome/multi-hit moves only hitting once with Sleep Talk
    • Test refactor for removing QueuedMove in favor of TurnMove (see 714211d)
  • Refactor Copycat/Mirror Move. (potentially make a CallMoveAttr, have all other attributes extend off of this one)
  • Refactor Promises into async/await pattern
  • Write unit tests for
    • Metronome
    • Assist
    • Sleep Talk
    • Copycat
    • Mirror Move

What are the changes?

Properly implements Assist.
Refactors RandomMovesetMoveAttr to extend RandomMoveAttr.
Creates arrays of invalid moves for each move: Assist, Sleep Talk, Metronome, since they each have different sets of uncallable moves.

Why am I doing these changes?

Assist did not call ally moves, had useless code, and was given no distinction in logic from Sleep Talk.
Metronome calls random moves in a similar manner to the two other moves, so it makes sense to have Assist/Sleep Talk reuse the code for calling moves.
None of Assist/Sleep Talk/Metronome had properly implemented lists of uncallable moves.

What did change?

  • Assist now uses the entire team's party
  • Assist/Sleep Talk's attribute now extends RandomMoveAttr and uses its callMoves function (which has been modified slightly to accommodate this) to call the move
  • Sleep Talk targeting is changed to NEAR_ENEMY
  • Copycat's last move used is now unaffected by moves that fail, see Copycat's bulbapedia
  • Mirror Move can now properly copy status moves
  • Mirror Move now targets a pokemon and copies that pokemon's last used move
  • Assist/Sleep Talk/Metronome/Copycat now have individual arrays listing uncallable moves. These arrays were created referencing Bulbapedia's list of uncallable moves.
  • Uncallable moves for Assist
  • Uncallable moves for Sleep Talk
  • Uncallable moves for Metronome
  • Uncallable moves for Copycat

Note: Sleep Talk specifies that it targets the opposite opponent. Since there is no MoveTarget specifically for opposite opponents, I chose to make it NEAR_ENEMY for now.

Screenshots/Videos

Metronome

Metronome.mp4

Assist

Assist.mp4

Sleep Talk

Sleep.Talk.mp4

How to test the changes?

Use src/overrides.ts to give yourself one of the three moves. If using Metronome, observe it will never call certain moves. If using Assist, observe it calling your party pokemon's moves. If using Sleep Talk, observe it only calling its own moveset.
Note that each move will no longer call certain moves: attempt Assist/Sleep Talk with 2 turn moves to see them fail.
List of moves that won't be callable are given above by links to bulbapedia.

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • Have I considered writing automated tests for the issue?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • Are the changes visual?
    • Have I provided screenshots/videos of the changes?

@schmidtc1 schmidtc1 marked this pull request as ready for review August 6, 2024 16:36
@schmidtc1 schmidtc1 requested a review from a team as a code owner August 26, 2024 15:14
src/data/move.ts Show resolved Hide resolved
frutescens
frutescens previously approved these changes Aug 28, 2024
Copy link
Collaborator

@frutescens frutescens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be helpful to have tests for these moves?

@schmidtc1
Copy link
Contributor Author

Do you think it would be helpful to have tests for these moves?

The only thing I could think to test is that they are not choosing invalid moves. Do you think this is necessary/have any edge cases to test for?

@Snailman11
Copy link
Collaborator

Snailman11 commented Aug 28, 2024

Hello, might be too late + disregard for the process, but could you consider adding this as part of your fix?
#3874 (Metronome and Copycat targeting allies)

@schmidtc1
Copy link
Contributor Author

Hello, might be too late + disregard for the process, but could you consider adding this as part of your fix? #3874 (Metronome and Copycat targeting allies)

I'll attempt a fix.

@flx-sta
Copy link
Collaborator

flx-sta commented Oct 2, 2024

@schmidtc1 please make sure to resolve your merge conflicts

@innerthunder innerthunder added the Refactor Rewriting existing code related label Oct 10, 2024
@DayKev DayKev added Enhancement New feature or request P2 Bug Minor. Non crashing Incorrect move/ability/interaction labels Oct 21, 2024
@schmidtc1 schmidtc1 marked this pull request as ready for review October 26, 2024 19:50
src/data/move.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Move Affects a move P2 Bug Minor. Non crashing Incorrect move/ability/interaction Refactor Rewriting existing code related
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

7 participants